-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attachment support #56
base: main
Are you sure you want to change the base?
Conversation
Only supports a single file. Doesn't generate a blurhash, so Mastodon seems to show the image as "Not available" for a few seconds.
This is super cool, thank you for this contribution. One thing I am wondering is if there is a very strong benefit to wrapping the media as base64 in a json file? Does it make it easier to handle in express? My concerns are that his will result in large files that can't be easily inspected. I would prefer to write them as normal files so that the owner of the instance can look at them without extra tooling. Also, this would allow them to be served out of a static media folder instead of processing them through a handler. Using the hash of the content to create the filename is nice since it elegantly handles duplicate files. Also, this will need a way to add the content description as well -- I am happy to work on that if you would prefer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again! See comments.
In my testing, the POST will fail for files that are larger than about 1 MB. Chrome debugger doesn't really show much - it says the request canceled. I suspect this has to do with Express hitting the payload size, but I'm not sure. Base64 stuff gets big fast. |
…ad, return immediately and handle the rest in .then() This was broken on Firefox, seemed ok in Chrome.
I agree about base64 encoded files getting big. I only chose to do it that way so that each individual media file on disk would have the binary data and the metadata (mimetype, + description, blurhash etc in future). |
I'm happy for you to do the content description, yes. |
Fix bug where sensitive was enabled by default (cw == undefined, not null)
I've just tested ok with an 8MB PNG (Firefox). Took a little while but worked. Initially it was failing due to nginx (https://www.cyberciti.biz/faq/linux-unix-bsd-nginx-413-request-entity-too-large/) |
@benbrown I'm done. Ready for you to try it. |
you are on fire @ringtailsoftware. I was just looking at your recent commits. I'll try it out when I can!! |
Why do we need to store the metadata separately from where it lives in the post itself? I think we get mime-type for free if the file gets written out to /media/hash.png |
@ringtailsoftware this is awesome. I might put some work into this while you sleep -- if the power doesn't go out in texas!! |
If we use the file extension then we'd still have to decode ".png" into "image/png" and so on. |
The original filename must be available in the browser - it could be passed as part of the payload, and then can use that to apply the extension. Ultimately I think it is important to have human readable files in the /media folder (meaning I can look at them with my normal tools) |
Ok, in that case, I'd suggest taking the mimetype "image/png" and renaming the data file to I'm concerned about someone uploading a file called "../../" or a PNG called "foo.jpg", etc. |
That'll do it |
…(eg. abc123.png). This means that the media directory could be served by a static webserver. When a request comes in via existing /media/:id endpoint, drop the .ext and lookup the mime type (for Content-Type) via the metadata.
Minor tweak made to allow media directory to be statically served |
I've added support for attaching a single file.
It was working here (with my other changes), until I merged with the latest composer changes.
Raising a PR in case it's obvious to you what's gone wrong, else I'll continue later...